-
Notifications
You must be signed in to change notification settings - Fork 378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #7029 - IP conversion report units #7032
Fix #7029 - IP conversion report units #7032
Conversation
…PConversionReport
@jmarrec I pulled the latest NREL/develop into this branch to get some fresh, cleaned up, CI results. I'll take a peek at this later today after CI does its runs. |
@@ -3462,7 +3462,7 @@ namespace HeatBalanceAirManager { | |||
if (Loop == 1) | |||
gio::write(OutputFileInits, Format_721) | |||
<< "ZoneInfiltration" | |||
<< "Design Volume Flow Rate {m3/s},Volume Flow Rate/Floor Area {m3/s/m2},Volume Flow Rate/Exterior Surface Area {m3/s/m2},ACH - " | |||
<< "Design Volume Flow Rate {m3/s},Volume Flow Rate/Floor Area {m3/s-m2},Volume Flow Rate/Exterior Surface Area {m3/s-m2},ACH - " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so m3/s-m2
is already the accepted unit conversion in E+, this output was simply entered incorrectly in this string. 👍
@@ -15351,7 +15351,7 @@ namespace OutputReportTabular { | |||
|
|||
// SUBROUTINE LOCAL VARIABLE DECLARATIONS: | |||
// na | |||
UnitConvSize = 115; | |||
UnitConvSize = 116; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problems here.
CI is looking happy with it so far. I'm building it locally and will mess around with it. |
Tested locally using 1ZoneEvapCooler.idf as the test file. Baseline branch:
This branch:
I don't see any red flags with this. I'll let CI finish the integration coverage, which is the longest of the tests, but I suspect in an hour or so when it's done with that, and the custom-check test, it'll be green and I'll merge this in. |
CI is super happy. Merging. |
Pull request overview
Fix #7029: Added "person/m2" to unitConv, and modified some units for eg
m3/s/m2
to bem3/s-m2
to be correctly picked up for conversion. Added the new unitConv to an existing Gtest.After changes it correctly converts the units (the before can be seen in #7029) (both label and conversion factor are ok).
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.